Install ovphysx wheel by default in --install#5780
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This is a well-structured PR that promotes the ovphysx wheel from opt-in to default installation, aligning with how the Newton wheel is already treated. The implementation is clean and the test updates are thorough.
✅ Strengths
-
Consistent design pattern: Following the existing Newton wheel approach makes the codebase more predictable and the install experience more uniform.
-
Improved test reliability: Removing the
importorskipguards means missing theovphysxwheel in CI will now fail loudly rather than silently skipping tests — this catches real regressions rather than masking them. -
Good documentation: Both changelog fragments clearly explain the change, and the help text in
__init__.pyis updated to reflect the new behavior. -
Comprehensive test updates: All 8 test files were updated to remove the skip guards, and
test_install_command_parsing.pyincludes a new test (test_all_installs_ov_with_default_ovphysx_selector) to verify the auto-install path.
💭 Minor Observations
-
No version pin on ovphysx (noted by author): The PR description mentions
setup.pyhas"ovphysx": ["ovphysx"]with no version constraint. This is fine for now, but consider adding a minimum version pin once a stable release is identified to prevent potential compatibility issues. -
CI job monitoring (noted by author): Since
isaaclab_ovwill now actually exercise the ovphysx tests, worth watching the first few CI runs to catch any previously-hidden issues. This is already acknowledged in the test plan.
🔍 Implementation Notes
The logic change in _install_ov_extra_dependencies() is clean:
- Empty selector → defaults to
{"ovphysx"} - Non-empty selector → validates and processes as before
- OVRTX remains opt-in via explicit
ov[ovrtx]orov[all]
The behavior for explicit selectors (ov[ovrtx], ov[all], ov[ovphysx]) is unchanged, which minimizes risk for users who already use those.
Overall this is a straightforward, well-tested change that improves the default user experience while maintaining backward compatibility for explicit install commands.
Update (commit c37a233): The PR direction has substantially changed — ovphysx is no longer auto-installed by --install all. Instead, ov is now in MANUAL_EXTRA_FEATURES and CI jobs explicitly install the wheel via extra-pip-packages. The importorskip guards have been re-added to all test files (graceful skip when wheel is absent).
Key changes in this iteration:
-
Reversed default-install approach —
ovmoved to manual extras;_install_ov_extra_dependencies()now prints a help message and returns early when called without a selector. This is a more conservative approach that avoids forcing the wheel on all users. -
New
device_splitpytest marker + CI infra —tools/_device_split.pyand a significant refactor oftools/conftest.pyto run marked test files once per device (CPU/GPU) in separate subprocesses, working around the ovphysx ≤0.3.7 process-global device lock. -
conftest.pyrefactor — The monolithicrun_individual_testsloop was cleanly decomposed into_run_one_pass()+_merge_pass_status()+_PassContextdataclass. Good separation of concerns. -
Test infrastructure —
tools/test_device_split.pyprovides solid unit test coverage for the new marker detection logic.
Assessment: The new direction is well-reasoned — keeping the wheel opt-in avoids download/install overhead for users who don't need it, while CI explicitly installs it for coverage. The device_split mechanism is a practical workaround for the ovphysx device-lock limitation and the refactoring of conftest.py improves maintainability. No new issues identified.---
Update (commit a47511b): Significant scope expansion. The PR now bundles multiple Newton, isaaclab_ov, and isaaclab_ovphysx improvements beyond the original ovphysx install change:
New changes reviewed:
-
Newton
projected_gravity_bkernels — Dedicated 1D/2D kernels that normalize per-env gravity on read, enabling per-env gravity randomization to propagate correctly.GRAVITY_VEC_Wnow binds directly tomodel.gravityinstead of copying env 0's value. Clean design. -
Timestamp guard for stale sensor data (#4970) — Contact sensor, IMU, PVA, and joint-wrench kernels now skip envs with
timestamp == 0.0(not stepped since reset). Prevents stale pre-reset data from leaking through. Each sensor has a corresponding regression test. -
collision_decimationconfig — Mid-tick re-collision support with proper gating (0= disabled, must be< num_substeps). Warning logged when misconfigured. Well-tested with counting mock. -
Per-world sites (
per_world=True) — Extendscl_register_sitefor bodyless sites per cloned world frame. Clean integration into_cl_inject_sites/_cl_inject_sites_fallback/newton_replicate. -
NewtonSiteFrameViewrefactor — Major simplification: removed duplicate non-indexed kernels, removed parent-relative local-pose machinery, added ClonePlan-aware site resolution. ~800 lines removed, cleaner architecture. -
Asset initialization cleanup — All three backends (Newton, OvPhysX, OvRTX) now use shared
resolve_matching_prims_from_source()+get_all_matching_child_prims()withexpected_num_matches=1instead of duplicated boilerplate. -
Legacy OVRTX 0.2.x removal — Removed
_IS_OVRTX_0_3_0_OR_NEWERgates, legacy kernels, temp-file fallback. Minimum is now 0.3.0. -
pyproject.tomlmodernization —isaaclab_newton,isaaclab_ov,isaaclab_ovphysxall migrated fromsetup.py+extension.tomlto declarativepyproject.toml. Clean.
Previous review concerns: Still addressed (no regressions from the c37a233 changes).
Assessment: No new issues found. The code is well-structured, properly tested (new regression tests for each behavioral change), and follows consistent patterns across the three packages.
Update (1fce150): The device_split pytest marker and its entire infrastructure (tools/_device_split.py, tools/test_device_split.py, the _PassContext/_run_one_pass/_merge_pass_status refactor in conftest.py) have been removed. The conftest.py is reverted to the simpler flat loop structure. The pytestmark = pytest.mark.device_split lines are removed from the two ovphysx test files, and the marker definition is dropped from pyproject.toml. The device-split changelog fragment is replaced with a .skip file.
This simplification suggests the ovphysx device-lock workaround is no longer needed (perhaps resolved upstream or the tests now handle device parametrization without separate-process isolation). The resulting conftest.py is cleaner and easier to maintain. No new issues introduced.
Update (2dba76d): Three new commits pushed (revert + changelog skip + restore). Net diff from previously reviewed SHA (a47511b4) is zero — the tree is identical. The device_split infrastructure, conftest refactor, and all test markers are back in place as previously reviewed. No new issues.
Update (6653d32): New commit adds Kit/runtime version logging to AppLauncher startup — a _log_kit_version_info() method prints Kit version, kernel version, and git hash to stderr for diagnostics. Accompanying changelog fragment added. Clean, no issues.
Update (5f232fb): Three additional test files now have the pytestmark = pytest.mark.device_split marker added:
isaaclab_ovphysx/test/assets/test_articulation.pyisaaclab_ovphysx/test/assets/test_rigid_object.pyisaaclab_ovphysx/test/assets/test_rigid_object_collection.py
This extends the device-split infrastructure to cover the ovphysx asset tests that were previously missing the marker. Consistent with the earlier device_split pattern — these files likely need separate CPU/GPU subprocess isolation due to the ovphysx process-global device lock. No issues.
Update (c746eaa): Minor improvements to the device_split infrastructure:
- ✅
is_device_split_file()now accepts an optionalsourceparameter to avoid redundant file I/O when the test content is already loaded inconftest.py. - ✅
conftest.pypasses the preloadedtest_contentto the function — eliminates a double-read. - ✅ New unit test
test_preloaded_sourcecovers the new parameter path. - ✅ Changelog fragment updated to list all 5 device-split test files (was previously 2).
Clean, no issues. Good housekeeping commit.
Greptile SummaryThis PR promotes the
Confidence Score: 4/5Safe to merge; install logic, dispatch tests, and guard removal are all internally consistent. The only thing worth a follow-up is a redundant module-level import left in one helper test file. The install-CLI changes are correct and well-covered by the updated test suite. The guard-removal sweep across the seven test files is clean — import pytest is dropped only in files that had no remaining pytest.* API calls, and pytest fixtures like monkeypatch/caplog are injected by the framework without a direct import. The one minor leftover is a duplicated MockOvPhysxBindingSet import inside test_mock_binding_set_rigid_object_shapes in test_articulation_helpers.py, which was pre-existing but is more visible now that the module-level import sits right above it. No functional defects found. source/isaaclab_ovphysx/test/assets/test_articulation_helpers.py — has a redundant inner import of MockOvPhysxBindingSet that mirrors the module-level import added by this PR. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["./isaaclab.sh --install [token]"] --> B{token?}
B -- "all / empty" --> C[Auto-install VALID_EXTRA_FEATURES minus MANUAL_EXTRA_FEATURES]
C --> D["newton, rl, visualizer, ov (empty selector)"]
D --> E["_install_ov_extra_dependencies('')"]
E --> F["selectors = {'ovphysx'}"]
F --> G["pip install isaaclab_ovphysx[ovphysx]"]
B -- "ov" --> E
B -- "ov[ovrtx]" --> H["_install_ov_extra_dependencies('ovrtx')"]
H --> I["selectors = {'ovrtx'}"]
I --> J["pip install isaaclab_ov[ovrtx]"]
B -- "ov[all]" --> K["_install_ov_extra_dependencies('all')"]
K --> L["selectors expanded to {'ovrtx','ovphysx'}"]
L --> G
L --> J
B -- "none" --> O["Core submodules only — no extras"]
B -- "contrib" --> P["MANUAL: requires explicit flag, never auto-installed"]
|
kellyguo11
left a comment
kellyguo11
left a comment
There was a problem hiding this comment.
is making ovphysx installable by default mostly for the CI tests? I feel like it could be kept as an optional dependency from user perspective, but we can force an install of it for our CI jobs?
Yes it's mostly for CI tests. Sure we could leave that optional and handle it similarly to OvRTX in the CI. |
Pure refactor; behavior unchanged. Enables the follow-up commit to invoke the helper twice per file for the device_split marker without duplicating the ~200-line retry/timeout/parse pipeline.
Resolves the ovphysx<=0.3.7 gap G5 device lock for any CI test file that opts in via 'pytestmark = pytest.mark.device_split'. Each pass gets its own subprocess, its own JUnit XML, and the merged counts are written under the original file key so the summary table is unchanged.
Avoids the ovphysx<=0.3.7 gap G5 device-lock failure surfaced by PR 5780 in the isaaclab_ov CI job. The conftest device_split runner splits this file into separate CPU and GPU subprocesses.
Avoids the ovphysx<=0.3.7 gap G5 device-lock failure on the test_no_contact_reporting case that runs after GPU-parametrized cases in the isaaclab_ov CI job.
Two tiny cleanups on the device_split CI machinery added in this PR: * Remove _PassContext.is_cold_cache_test — set by the caller but never read by _run_one_pass; the cold-cache buffer is applied to timeout and startup_deadline before the dataclass is built, so the field was purely informational. * Add a presence check before appending to failed_tests so a device_split file failing on both CPU and GPU passes only appears once in the 'failed tests:' debug print (exit code is unaffected; it derives from test_status keys).
Install ovphysx explicitly in CI jobs that exercise OV coverage instead of adding it to the default user install path. Restore optional OVPhysX test guards so local and partial installs skip when the runtime wheel is absent.
c37a233 to
a47511b
Compare
|
Review Update (commit 2dba76d): The What's back:
Assessment: The restoration is clean and complete. The implementation is sound — detecting the marker via regex on the source file avoids import-time side effects, and the two-pass No new issues found. The PR is in good shape for final human review. |
|
Update (dbd83a4): Latest commit improves CI summary visibility for
Assessment: Clean enhancement. The additional |
|
@myurasov-nv @hujc7 any opinions on the auto split cpu/gpu device. |
This is great. The only concern is that it sits more in the test logic as well as the retries, which is not always obvious at test level (I think it's mostly for the process lock issue?). I also have something in #5823 which more relies on Btw, I saw ovphysx has newer version. Is the process-global device lock still a problem? |
Thanks for taking a look! About 5823 the difference here is the device lock, we cannot run the cpu and gpu tests in the same run, we need to launch two separate test instances to test on all the devices. This is why it sits there. It could also be hard-coded, but that sounds like a bad idea? Yeah, the new version will still have the device lock. I don't think they are planning on removing the lock at all. |
|
|
||
| * Re-enabled both CPU and GPU coverage in CI for | ||
| :file:`test/sim/test_views_xform_prim_ovphysx.py` and | ||
| :file:`test/sensors/test_contact_sensor.py` by tagging them with the new |
There was a problem hiding this comment.
nit: some more files were tagged with that
There was a problem hiding this comment.
Updated in c746eaa: the changelog now lists all five OVPhysX test files tagged with device_split.
|
|
||
| timeout = test_settings.PER_TEST_TIMEOUTS.get(file_name, test_settings.DEFAULT_TIMEOUT) | ||
|
|
||
| # Read the test file once for cold-cache and device-split detection. |
There was a problem hiding this comment.
I think test_content is read below and also in is_device_split_file, could be worth passing the contents to save one read
There was a problem hiding this comment.
Done in c746eaa: is_device_split_file now accepts preloaded source text, and tools/conftest.py passes the test_content it already read. Added a unit test for that path.
| isaacsim-base-image: ${{ needs.config.outputs.isaacsim_image_name }} | ||
| isaacsim-version: ${{ needs.config.outputs.isaacsim_image_tag }} | ||
| filter-pattern: "isaaclab_ov" | ||
| extra-pip-packages: "ovrtx ovphysx" |
There was a problem hiding this comment.
uhm the installs could fail but CI might keep going with those importorskip, no? and quietly telling a green outcome. Just confirming this is the intended behavior.
There was a problem hiding this comment.
Confirmed: the extra package install runs inside the Docker test command under set -e, and .github/actions/run-tests/action.yml returns the container exit code after copying reports. So if pip install ovphysx fails, the job fails before pytest/importorskip can produce a green result. The importorskip guards are only for local or partial installs where the optional wheel was not requested.
Pass preloaded test source into the device_split detector so the per-file runner does not read the same test twice. Update the OVPhysX changelog fragment to list every test file now tagged for split CPU and GPU coverage.
4 participants
Description
Promote the publicly available
ovphysxPyPI wheel out of the manualov[ovphysx]extra and into the default./isaaclab.sh --installflow, mirroring how the newton wheel is treated. The OVRTX renderer wheel stays opt-in via--install 'ov[ovrtx]'or--install 'ov[all]'.Concretely:
MANUAL_EXTRA_FEATURESdrops"ov"(it now only contains"contrib"), so theovfeature is part of the automatic-i/-i allset alongsidenewton,rl, andvisualizer._install_ov_extra_dependencies("")no longer prints a help message and returns; it now installsisaaclab_ovphysx[ovphysx]. Explicitov[ovrtx],ov[ovphysx], andov[all]behavior is unchanged.--installhelp text and thecommand_installdocstring are updated to reflect the new default.pytest.importorskip("ovphysx.types", reason="ovphysx wheel not installed")guards (9 sites) and one innerimportorskip("isaaclab_ovphysx.tensor_types")call are removed from theisaaclab_ovphysxtest suite. Theisaaclab_ovCI job (whosefilter-pattern: "isaaclab_ov"already collects these tests) now exercises them for real instead of silently skipping.No new required dependency is added;
source/isaaclab_ovphysx/setup.pyalready declaredEXTRAS_REQUIRE = {"ovphysx": ["ovphysx"]}. The extra remains unpinned for now — pinning a minimum version is a sensible follow-up once we lock in a release.Fixes # (n/a)
Type of change
Strictly, the default install footprint grows by one PyPI wheel and previously-skipped tests now run, so downstream consumers of
--installsee new behavior — but no public API is removed or renamed and all existingov[...]selectors continue to work as before.Screenshots
N/A — install-CLI and test-gating change.
Checklist
pre-commitchecks with./isaaclab.sh --format--helptext andcommand_installdocstring updated)test_all_installs_ov_with_default_ovphysx_selectorplus updates totest_manual_extra_featuresandtest_all_does_not_install_manual_extra_dependencies; 51/51 pass intest_install_command_parsing.py)source/<pkg>/changelog.d/for every touched package —source/isaaclab/changelog.d/antoiner-ovphysx-default-install.rstandsource/isaaclab_ovphysx/changelog.d/antoiner-ovphysx-default-install.rstCONTRIBUTORS.mdor my name already exists there